-
Notifications
You must be signed in to change notification settings - Fork 3k
ci: add strict-no-cover to detect unnecessary coverage pragmas #1897
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
78d6633 to
edefce3
Compare
| uv run --frozen --no-sync coverage report | ||
| - name: Check for unnecessary no cover pragmas | ||
| if: runner.os != 'Windows' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Kludex without this all the windows pipelines fail to even run, seems like a strict-no-cover bug? Example failure from previous CI run: https://github.com/modelcontextprotocol/python-sdk/actions/runs/21070443439/job/60598385217
|
Is this working already? All the pragmas are actually being used? |
not sure, will test |
|
@Kludex Found two issues that prevented 1. Coverage 7.13.0 changed Fix: pin 2.
Fix: remove I also added a test |
Add strict-no-cover from pydantic to CI pipeline. This tool identifies `# pragma: no cover` comments on lines that are actually covered by tests, helping keep coverage pragmas accurate. Changes: - Add strict-no-cover as dev dependency (installed from git) - Add `pragma: lax no cover` to coverage exclude_lines for partial coverage - Add CI step to run strict-no-cover after coverage report (Linux only due to Windows bug in the tool)
Two issues prevented strict-no-cover from detecting unnecessary pragmas: 1. Coverage 7.13.x changed analysis_from_file_reporter to filter executed lines with '& statements', which removes excluded lines from the executed set. This breaks strict-no-cover's detection (it checks the intersection of excluded_lines and executed_lines). Pin to <7.13. 2. relative_files = true causes coverage to store relative paths, but strict-no-cover creates a temp rcfile without [tool.coverage.run] settings. Without relative_files in that temp config, coverage can't match paths, resulting in empty executed_lines. Also adds a test pragma on a covered line to verify the check fails.
464a9ff to
e942316
Compare
Remove ~100 unnecessary '# pragma: no cover' annotations from 30 test files. These lines are actually covered by tests, as detected by strict-no-cover in CI.
Restore # pragma: no cover and # pragma: no branch on lines that are not actually covered in CI (platform-dependent, branch coverage, or unreachable code paths).
…laky tests - Remove unnecessary '# pragma: no cover' from 15 src/ files where lines are confirmed covered by CI - Switch flaky test pragmas to '# pragma: lax no cover' for lines that are sometimes covered and sometimes not across CI runs
…d ones - Remove ~80 unnecessary '# pragma: no cover' from src/ files where lines are confirmed covered by CI - Restore pragmas with correct type (no cover vs no branch) for lines with missing coverage or branch coverage in batch 1 files
Remove or adjust coverage pragmas in src/ files that were flagged by strict-no-cover as covering already-tested code. Changes: - Remove pragmas from lines/blocks that are fully covered by tests - Change '# pragma: no cover' to '# pragma: no branch' on lines where the condition itself is executed but one branch isn't taken - Move pragmas from except clause headers to their uncovered bodies where the except IS matched but the handler body isn't reached - Add targeted pragmas to genuinely uncovered code paths (error handlers, rarely-hit branches, cleanup paths) Files modified: 16 src/ files across cli, client, server, and shared
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
Replace incorrect pragma types with the appropriate ones: - Use 'lax no cover' for covered lines with uncovered bodies (excluded from coverage but not checked by strict-no-cover) - Use 'no branch' for branches where target lines are covered but the branch path itself is not taken - Keep 'no cover' only for genuinely uncovered code paths
Add strict-no-cover from pydantic to the CI pipeline. This tool identifies
# pragma: no covercomments on lines that are actually covered by tests, helping keep coverage pragmas accurate.Changes
pragma: lax no coverto coverage exclude_lines for partial coverage scenariosHow it works
# pragma: no cover- errors if any marked lines are actually covered# pragma: lax no cover- permits partial coverage without erroring (useful for flaky or inconsistently covered code)